Skip to content

feat: support Utf8View and BinaryView #5685

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
lichuang wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: support Utf8View and BinaryView #5685

lichuang wants to merge 3 commits into from

Conversation

@lichuang
Copy link
Contributor

@lichuang lichuang commented Jan 11, 2026

close #5172 , add support of Utf8View and BinaryView, test these two types in test_query_string

@github-actions github-actions bot added the enhancement New feature or request label Jan 11, 2026
@lichuang
Copy link
Contributor Author

lichuang commented Jan 12, 2026 •
edited
Loading

It seems that the compilation error(https://github.com/lance-format/lance/actions/runs/20890689101/job/60043036449?pr=5685) has nothing to do with the modifications made in this pr. @wjones127

@codecov
Copy link

codecov bot commented Jan 13, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 91.62562% with 51 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance-index/src/scalar/bloomfilter.rs 87.76% 13 Missing and 4 partials ⚠️
rust/lance-encoding/src/data.rs 93.61% 15 Missing ⚠️
...coding/src/previous/encodings/logical/primitive.rs 95.88% 6 Missing and 1 partial ⚠️
rust/lance-core/src/datatypes.rs 0.00% 2 Missing and 2 partials ⚠️
rust/lance-datafusion/src/expr.rs 0.00% 3 Missing ⚠️
rust/lance-arrow/src/lib.rs 96.15% 2 Missing ⚠️
rust/lance-core/src/datatypes/field.rs 60.00% 2 Missing ⚠️
rust/lance-encoding/src/decoder.rs 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

wjones127
wjones127 requested changes Jan 13, 2026
View reviewed changes
Copy link
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be able to accomplish this without having to convert in so many places.

Also, could you validate FTS indices build correctly? I think one function you'll need to update is here:

https://github.com/lance-format/lance/blob/5513248362b25daa1c381ef4bfb0da0939a8f4d0/rust/lance-arrow/src/lib.rs#L458-L464

@lichuang lichuang force-pushed the support-utf8-view branch 2 times, most recently from 73f8249 to 29c016b Compare January 14, 2026 14:49
@lichuang
Copy link
Contributor Author

lichuang commented Jan 24, 2026

@wjones127 please review this pr again

wjones127
wjones127 reviewed Jan 26, 2026
View reviewed changes
Copy link
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes looking good so far.

Note that before we merge this change, we'll need to change the spec to add these new data types first. So the next steps (which I've started are):

  • Post a discussion about the changes #5817
  • Vote on the spec changes (1 week voting period)
  • If the vote passes, merge the spec changes
  • Merge this PR

rust/lance-encoding/src/data.rs Outdated
Comment on lines 587 to 590
// Check if we need to convert offsets from i32 to i64 (for LargeUtf8/LargeBinary)
let offsets_buffer = if self.bits_per_offset == 32
&& (data_type == DataType::LargeUtf8 || data_type == DataType::LargeBinary)
{
Copy link
Contributor

@wjones127 wjones127 Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary?

Copy link
Contributor Author

@lichuang lichuang Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wjones127

The offsets conversion logic at lines 587-590 is necessary for backward compatibility. It handles the case when reading legacy data that was stored with 32-bit offsets but is now being read as LargeUtf8/LargeBinary (which use 64-bit offsets).

This is not introduced by this PR - it's existing code to handle historical data formats. I've added more detailed comments to clarify this purpose.

For Utf8View and BinaryView types in this PR, this conversion is not triggered because we store them with 64-bit offsets (as implemented in arrow_view_to_data_block).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wjones127 wjones127 wjones127 requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add support for StringView and BinaryView

2 participants

@lichuang @wjones127